Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ruby 3.1: Add specs for Time#new in keyword #956

Merged
merged 3 commits into from
Oct 14, 2022
Merged

Ruby 3.1: Add specs for Time#new in keyword #956

merged 3 commits into from
Oct 14, 2022

Conversation

ohbarye
Copy link
Contributor

@ohbarye ohbarye commented Oct 8, 2022

Issue

Change

This pull request covers the spec below.

  • Time.new now accepts optional in: keyword argument for the
    timezone, as well as Time.at and Time.now, so that is now
    you can omit minor arguments to Time.new. [Feature #17485]

    Time.new(2021, 12, 25, in: "+07:00")
#=> 2021-12-25 00:00:00 +0700

    At the same time, time component strings are converted to
    integers more strictly now.

    Time.new(2021, 12, 25, "+07:30")
#=> invalid value for Integer(): "+07:30" (ArgumentError)

    Ruby 3.0 or earlier returned probably unexpected result
    2021-12-25 07:00:00, not 2021-12-25 07:30:00 nor
    2021-12-25 00:00:00 +07:30.

I used test cases for Time.at as a reference.

@eregon
Copy link
Member

eregon commented Oct 9, 2022

Thank you, could you also add specs for Time.at(..., in:) and Time.now(in: ) since that's part of the same item?

@ohbarye ohbarye force-pushed the add-specs-for-time-new-in-keyword branch from 8736649 to 6b1c28a Compare October 9, 2022 13:25
@ohbarye
Copy link
Contributor Author

ohbarye commented Oct 9, 2022

Sure thing. How about fe751cb and 6b1c28a?

ohbarye
ohbarye commented Oct 9, 2022
View reviewed changes
core/time/now_spec.rb
@@ -3,4 +3,49 @@

describe "Time.now" do
it_behaves_like :time_now, :now

describe ":in keyword argument" do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I first thought those spec could be put into core/time/shared/now.rb as shared examples. But the in keyword behavior differs between Time.now and Time.new by Ruby version, so I just wrote these specs without ruby_version_is here.

eregon
eregon approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@eregon eregon merged commit 96fa596 into ruby:master Oct 14, 2022
@ohbarye ohbarye deleted the add-specs-for-time-new-in-keyword branch October 15, 2022 01:24
@ohbarye
Copy link
Contributor Author

ohbarye commented Oct 15, 2022

Thank you too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ohbarye @eregon